fix(dsl): isolate partial recursion guards - #2489
Conversation
ErenAta16
left a comment
There was a problem hiding this comment.
This is done correctly, including the part that's easy to get wrong.
You used default=None rather than a mutable default, and established the guard with .set(set()) plus reset(token). That matters more than it looks: ContextVar("...", default=set()) followed by .get().add(...) would have compiled, read naturally, and still leaked, because context copies are shallow so every scope that never called .set() mutates the same default object. Sidestepped cleanly here.
The placement is also right. Establishing the guard only when get() is None means the outermost call owns it and nested _process_generic_arg calls reuse the existing set, so the self-referential recursion guard still does its job. Creating a fresh set per call would have silently disabled cycle detection for TreeNode-style models, which is the failure mode I'd have looked for first.
Verified the isolation holds on both concurrency models, using the same guard shape as the patch:
threads : Home -> Address wrapped | Office -> Address wrapped
asyncio : Home -> Address wrapped | Office -> Address wrapped
And confirmed the test isn't vacuous by replaying the old global-set logic under the same interleaving:
thread 1 adds Address to the global set and blocks inside _make_partial_type
thread 2 arrives -> sees Address already present -> returns raw Address
which is exactly what makes "PartialAddress" in office_partial.model_json_schema()["$defs"] fail on main. Good choice of assertion, checking the $defs of the second model is what distinguishes "guard working" from "guard stomped", where a same-model test would pass either way.
Two optional notes, neither blocking:
The test covers ThreadPoolExecutor. The asyncio path is the one instructor's own streaming code actually hits (client.py does response_model = Partial[response_model] per call), and it isolates correctly, I checked. Worth a second test with asyncio.gather if you want the coverage to match the real call pattern, though the mechanism is identical so it's belt-and-braces.
assert processing_models is not None reads as a runtime guard but is a type-narrowing hint for the checker, and assert is stripped under python -O. That's harmless here since the preceding branch guarantees it's set, just noting it isn't load-bearing in case someone later reads it as one.
Nice turnaround on this one.
|
Consolidated and shipped in #2495. Closing this focused patch as superseded; thank you for the contribution. |
What\n\nPrevents concurrent partial-model construction from sharing recursion-detection state.\n\nCloses #2461.\n\n## Changes\n\n- Replace the module-global recursive-model guard with context-local state.\n- Scope guard lifecycle to each top-level partial-model construction.\n- Add a deterministic two-thread regression test.\n- Document the fix in the changelog.\n\n## Testing\n\n- uv run pytest tests/coverage/test_dsl_partial_coverage.py\n- uv run ruff check instructor/v2/dsl/partial.py tests/coverage/test_dsl_partial_coverage.py\n- uv run ruff format --check instructor/v2/dsl/partial.py tests/coverage/test_dsl_partial_coverage.py\n- uv run ty check instructor/v2/dsl/partial.py